Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE/IDENTITY] Add bearer authentication to feature/identity #5611

Merged

Conversation

stephen-crawford
Copy link
Contributor

Signed-off-by: Stephen Crawford [email protected]

Description

Adds bearer authentication support to the feature/identity branch of OpenSearch core. This implementation is built off of the existing Basic Authentication framework and includes testing.

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@stephen-crawford stephen-crawford changed the title Add bearer authentication to feature/identity [FEATURE/IDENTITY] Add bearer authentication to feature/identity Dec 21, 2022
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@stephen-crawford
Copy link
Contributor Author

@DarshitChanpura @cwperks This finally passed, but I am not thrilled about the license setup that I had to do. I thought that we could share licenses across modules so that authn and identity could use the same license but I could not get that to work so eventually just did it manually. If either of you know how to fix the sharing configuration let me know. Otherwise this should be all set. I can also squash the commits when doing this.

@cwperks
Copy link
Member

cwperks commented Dec 28, 2022

@scrawfor99 It depends on which build.gradle file the dependency is in. The sha, LICENSE and NOTICE files should be in the build.gradle file of the project where the corresponding implementation or api dependency line is located.

In our case the identity module depends on the opensearch-authn library so since the cxf dependencies are in identity that's where the files should be so what you have done looks good to me.

For anyone reading this and wondering why there is both identity and opensearch-authn, its first had to do with how feature/identity has changed over time as we figure out a way to sandbox the changes and get ready for a merge with main down the line. feature/identity started as classes in an identity package under server, but then moved to sandbox where experimental code should go before being promoted to a main feature. Some code in the package migrated to the sandbox/opensearch-authn lib and the lib was added as a dependency to server so that the classes were importable. Most recently, the identity module was introduced and the opensearch-authn dependency on server was removed to keep the changes in this feature branch isolated from the server. opensearch-authn may be removed in the future and the classes there moved into the identity module. There may be a library introduced in the future for other modules and plugins to use identity features conveniently.

@stephen-crawford
Copy link
Contributor Author

Hi @cwperks, thank you for the explanation. I was not sure what the difference between the two modules was myself so I appreciate you clearing that up. I will say that the dependencies are listed in both authn and identity--which seems like a mistake on my part given your explanation. I am going to remove them from identity and swap the api tag in the identity build.gradle over to an implementation as this should make the module inherit the transitive dependencies in authn.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Add bearer authentication to feature/identity

Signed-off-by: Stephen Crawford <[email protected]>

Update documentation

Signed-off-by: Stephen Crawford <[email protected]>

Spotless apply Jenkins fix

Signed-off-by: Stephen Crawford <[email protected]>

Add license file

Signed-off-by: Stephen Crawford <[email protected]>

Add sha

Signed-off-by: Stephen Crawford <[email protected]>

Rename License File

Signed-off-by: Stephen Crawford <[email protected]>

Add empty notice file

Signed-off-by: Stephen Crawford <[email protected]>
stephen-crawford and others added 4 commits December 28, 2022 15:31
Signed-off-by: Stephen Crawford <[email protected]>
Adds support for BearerToken for InternalRealm and skips credential matching for jwtToken

Signed-off-by: Darshit Chanpura <[email protected]>

Modifies bearer auth tests to test correct response

Signed-off-by: Darshit Chanpura <[email protected]>

Push before rebase

Signed-off-by: Stephen Crawford <[email protected]>

Fix public change

Signed-off-by: Stephen Crawford <[email protected]>

rename test case

Signed-off-by: Stephen Crawford <[email protected]>

Fix exceptions

Signed-off-by: Stephen Crawford <[email protected]>

remove unneeded method

Signed-off-by: Stephen Crawford <[email protected]>

Spotless

Signed-off-by: Stephen Crawford <[email protected]>

Remove old exception throw

Signed-off-by: Stephen Crawford <[email protected]>

Fix failing test

Signed-off-by: Stephen Crawford <[email protected]>

Remove throw from Subject

Signed-off-by: Stephen Crawford <[email protected]>

Rename fail statement

Signed-off-by: Stephen Crawford <[email protected]>

spotless

Signed-off-by: Stephen Crawford <[email protected]>

Remove dead throw and split into utility class

Signed-off-by: Stephen Crawford <[email protected]>

Add javadocs for utility methods

Signed-off-by: Stephen Crawford <[email protected]>

Changes requested

Signed-off-by: Stephen Crawford <[email protected]>

retry

Signed-off-by: Stephen Crawford <[email protected]>

Try to resolve build issue

Signed-off-by: Stephen Crawford <[email protected]>

Update shas

Signed-off-by: Stephen Crawford <[email protected]>

Update licenses

Signed-off-by: Stephen Crawford <[email protected]>

Retry license fix

Signed-off-by: Stephen Crawford <[email protected]>

Retry licenses

Signed-off-by: Stephen Crawford <[email protected]>

Spotless

Signed-off-by: Stephen Crawford <[email protected]>

Remove old sha

Signed-off-by: Stephen Crawford <[email protected]>

Add basic into identity dependencies

Signed-off-by: Stephen Crawford <[email protected]>

Update SHAs

Signed-off-by: Stephen Crawford <[email protected]>

Add licenses

Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scrawfor99 Thank you for addressing PR comments. Looks like there are some unintended changes in identity/build.gradle. Can you please address those?

sandbox/modules/identity/build.gradle Outdated Show resolved Hide resolved
sandbox/modules/identity/build.gradle Show resolved Hide resolved
sandbox/modules/identity/build.gradle Outdated Show resolved Hide resolved
MatcherAssert.assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(401));
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwperks @scrawfor99 This test suite seems like a mix of Unit (lines 32-91) and Integration (lines 93-159) tests. Should we separate them out into two classes. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever people think is best seems fine with me. I see why you would separate them by test type, but I also get why keeping all the tests related to Bearer Auth stuff together is nice. Whichever is preferred I can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further review, I think it is best to leave things as-is for now. The primary reason being the dependency handling between authn and identity. Right now, identity is dependent on authn but all of the JwtVendor* classes are in identity. In order to move the unit tests over to the AuthenticationTokenHandlerTests class we would need to make authn dependent on identity or move the AuthenticationTokenHandlerTests.
If we end up combining authn and identity then this will no longer be an issue but for the time being it seems best to merge as-is and revisit down the road.

Signed-off-by: Stephen Crawford <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.action.admin.indices.create.ShrinkIndexIT.testCreateShrinkIndex

@DarshitChanpura
Copy link
Member

@peternied Can you please review and merge this change once you get a chance?

@peternied
Copy link
Member

@cwperks I'm not sure if all your comments were fully addressed, happy to review follow up PRs or follow up issues if you'd still like to see more changes.

@peternied peternied merged commit c39a92b into opensearch-project:feature/identity Jan 3, 2023
@stephen-crawford
Copy link
Contributor Author

@peternied, I believe all of @cwperks have been addressed. The only unresolved one that I am aware of is the potential merging of the identity and authn substructures which will require a separate initiative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants